Found on Ubuntu 16.04 with m-c 20170614153332 da66c4a05f This test case requires fuzzPriv plugin. ==7247==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030004f2210 at pc 0x7f9fdc96087a bp 0x7ffdeb0d04d0 sp 0x7ffdeb0d04c8 READ of size 8 at 0x6030004f2210 thread T0 #0 0x7f9fdc960879 in ~RefPtr src/obj-firefox/dist/include/mozilla/RefPtr.h:77:9 #1 0x7f9fdc960879 in Destruct src/obj-firefox/dist/include/nsTArray.h:560 #2 0x7f9fdc960879 in DestructRange src/obj-firefox/dist/include/nsTArray.h:2012 #3 0x7f9fdc960879 in RemoveElementsAt src/obj-firefox/dist/include/nsTArray.h:2060 #4 0x7f9fdc960879 in mozilla::a11y::DocAccessible::PutChildrenBack(nsTArray<RefPtr<mozilla::a11y::Accessible> >*, unsigned int) src/accessible/generic/DocAccessible.cpp:2187 #5 0x7f9fdc95f56d in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2136:3 #6 0x7f9fdc8cee8c in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:811:18 #7 0x7f9fd96e4ee4 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1831:12 #8 0x7f9fd96f4455 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301:7 #9 0x7f9fd96f4112 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:322:5 #10 0x7f9fd96f683b in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:754:5 #11 0x7f9fd96f683b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:667 #12 0x7f9fd96f1b17 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:513:20 #13 0x7f9fd2de3798 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1416:14 #14 0x7f9fd2df0678 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10 #15 0x7f9fd3b9d221 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21 #16 0x7f9fd3afb3a0 in RunInternal src/ipc/chromium/src/base/message_loop.cc:318:10 #17 0x7f9fd3afb3a0 in RunHandler src/ipc/chromium/src/base/message_loop.cc:311 #18 0x7f9fd3afb3a0 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:291 #19 0x7f9fd90519ef in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27 #20 0x7f9fdd0ce431 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30 #21 0x7f9fdd29e674 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22 #22 0x7f9fdd2a01e0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8 #23 0x7f9fdd2a1531 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21 #24 0x4eb613 in do_main src/browser/app/nsBrowserApp.cpp:237:22 #25 0x4eb613 in main src/browser/app/nsBrowserApp.cpp:310 #26 0x7f9fef27282f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291 #27 0x41d168 in _start (m-c-1497454412-asan-opt/firefox+0x41d168) 0x6030004f2210 is located 0 bytes to the right of 32-byte region [0x6030004f21f0,0x6030004f2210) allocated by thread T0 here: #0 0x4bbd9e in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:77:3 #1 0x4ed0bd in moz_xrealloc src/memory/mozalloc/mozalloc.cpp:105:20 #2 0x7f9fd2bad1a6 in Realloc src/obj-firefox/dist/include/nsTArray.h:212:12 #3 0x7f9fd2bad1a6 in nsTArrayInfallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned long, unsigned long) src/obj-firefox/dist/include/nsTArray-inl.h:183 #4 0x7f9fdc95fab2 in RefPtr<mozilla::a11y::Accessible>* nsTArray_Impl<RefPtr<mozilla::a11y::Accessible>, nsTArrayInfallibleAllocator>::InsertElementAt<mozilla::a11y::Accessible*&, nsTArrayInfallibleAllocator>(unsigned long, mozilla::a11y::Accessible*&) src/obj-firefox/dist/include/nsTArray.h:2142:47 #5 0x7f9fdc95f31c in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2081:21 #6 0x7f9fdc8cee8c in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:811:18 #7 0x7f9fd96e4ee4 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1831:12 #8 0x7f9fd96f4455 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301:7 #9 0x7f9fd96f4112 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:322:5 #10 0x7f9fd96f683b in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:754:5 #11 0x7f9fd96f683b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:667 #12 0x7f9fd96f1b17 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:513:20 #13 0x7f9fd2de3798 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1416:14 #14 0x7f9fd2df0678 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10 #15 0x7f9fd3b9d221 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21 #16 0x7f9fd3afb3a0 in RunInternal src/ipc/chromium/src/base/message_loop.cc:318:10 #17 0x7f9fd3afb3a0 in RunHandler src/ipc/chromium/src/base/message_loop.cc:311 #18 0x7f9fd3afb3a0 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:291 #19 0x7f9fd90519ef in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27 #20 0x7f9fdd0ce431 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30 #21 0x7f9fdd29e674 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22 #22 0x7f9fdd2a01e0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8 #23 0x7f9fdd2a1531 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21 #24 0x4eb613 in do_main src/browser/app/nsBrowserApp.cpp:237:22 #25 0x4eb613 in main src/browser/app/nsBrowserApp.cpp:310 #26 0x7f9fef27282f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291 SUMMARY: AddressSanitizer: heap-buffer-overflow src/obj-firefox/dist/include/mozilla/RefPtr.h:77:9 in ~RefPtr Shadow bytes around the buggy address: 0x0c06800963f0: 00 00 06 fa fa fa ac 00 00 fa fa fa 00 00 06 fa 0x0c0680096400: fa fa ac 00 00 fa fa fa 00 00 06 fa fa fa fa fa 0x0c0680096410: fa fa fa fa ac 00 00 fa fa fa 00 00 00 00 fa fa 0x0c0680096420: ac 00 00 fa fa fa fa fa fa fa fa fa 00 00 00 fa 0x0c0680096430: fa fa 00 00 00 fa fa fa 00 00 00 00 fa fa 00 00 =>0x0c0680096440: 00 00[fa]fa 00 00 00 00 fa fa ac 00 00 fa fa fa 0x0c0680096450: 00 00 00 03 fa fa fd fd fd fd fa fa 00 00 00 fa 0x0c0680096460: fa fa 00 00 00 00 fa fa ac 00 00 fa fa fa fd fd 0x0c0680096470: fd fd fa fa 00 00 00 fa fa fa fd fd fd fd fa fa 0x0c0680096480: ac 00 00 fa fa fa fd fd fd fd fa fa 00 00 00 00 0x0c0680096490: fa fa 00 00 00 fa fa fa fd fd fd fd fa fa ac 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==7247==ABORTING

I hit an assertion in debug build when running the test, taking it.

On the XPCOM side it seems like maybe we should add a release bounds check to |RemoveElementsAt| [1], Nathan wdyt? It looks like we're probably calling a dtor on junk bytes due to an out of bounds index. [1] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/xpcom/ds/nsTArray.h#2052-2064

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #2) > On the XPCOM side it seems like maybe we should add a release bounds check > to |RemoveElementsAt| [1], Nathan wdyt? It looks like we're probably calling > a dtor on junk bytes due to an out of bounds index. That sounds like an excellent idea to me.

Comment on attachment 8878242 [details] [diff] [review] patch Review of attachment 8878242 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +2067,3 @@ > while (nsIContent* childEl = iter.NextElem()) { > Accessible* child = GetAccessible(childEl); > + auto insertIdx = aOwner->ChildCount() - owned->Length() + idx; a small comment I missed yesterday evening when posted the patch: that's the idea of the patch, we cannot rely on IndexInParent() to track the insertion index, because an accessible may alter insertion point, and thus we mess up. That's happens with caption inside table. ::: dom/base/nsTextFragment.h @@ +199,5 @@ > * index. This always returns a char16_t. > */ > char16_t CharAt(int32_t aIndex) const > { > + MOZ_ASSERT(uint32_t(aIndex) < mState.mLength, "bad index"); this one is artifact and will be removed

Comment on attachment 8878242 [details] [diff] [review] patch Review of attachment 8878242 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +2067,3 @@ > while (nsIContent* childEl = iter.NextElem()) { > Accessible* child = GetAccessible(childEl); > + auto insertIdx = aOwner->ChildCount() - owned->Length() + idx; ChildCount already includes the relocated children? If not (and I don't see how it does), I would expect insertIdx == (ChildCount() + idx). EDIT: Oh, 'owned' is stored from our previous relocation, not the current attribute value. nm @@ +2079,5 @@ > imut.Done(); > > child->SetRelocated(true); > + owned->InsertElementAt(idx, child); > + idx++; nit here and below: owned->InsertElementAt(idx++, child) Unless we explicitly avoid that kind of pattern?

(In reply to Eitan Isaacson [:eeejay] from comment #6) > > + auto insertIdx = aOwner->ChildCount() - owned->Length() + idx; > > ChildCount already includes the relocated children? If not (and I don't see > how it does), I would expect insertIdx == (ChildCount() + idx). > > EDIT: Oh, 'owned' is stored from our previous relocation, not the current > attribute value. nm mChildren contains both explicit and ARIA owned children (TreeWalker takes care of it). The ARIA owns hash ('owned' array) contains an array of ARIA owned children only, 'idx' is responsible to track number of iterations. As we iterate over ARIA owned children, then we update mChildren and owned children arrays, so mChildren.Length() - owned->Length() always point to an index where ARIA owned children start from. > > child->SetRelocated(true); > > + owned->InsertElementAt(idx, child); > > + idx++; > > nit here and below: owned->InsertElementAt(idx++, child) > > Unless we explicitly avoid that kind of pattern? I used to prefer this syntax myself, but I've been told number of times that some people find it less readable and less error-prone, so I finally switched to that syntax, not sure whether this is good or bad.

Comment on attachment 8878242 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? not easy, good moz a11y knowledge is required Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no, I would go with something like "ARIA owned children ordering may be incorrect" Which older supported branches are affected by this flaw? all Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? should be same How likely is this patch to cause regressions; how much testing does it need? needs some cycles on nightly, to give our QA and community enough time to catch the problems

sec-approval+ for trunk. We'll want patches nominated for Beta and ESR52 as well.

Can we get this checked into trunk and Beta and ESR52 patches nominated?

This'll need rebased patches for Beta & ESR52.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > This'll need rebased patches for Beta & ESR52. it depends on bug 1369836, which is not yet landed on beta & esr52.

Indeed, this grafts cleanly to Beta now. Please request approval when you get a chance.

Comment on attachment 8878242 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]:unknown [User impact if declined]:crash, sec issue [Is this code covered by automated tests?]:fair code coverage [Has the fix been verified in Nightly?]:yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]:no [Is the change risky?]:moderate risk [Why is the change risky/not risky?]:changes in code known lately for its instability [String changes made/needed]:no

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > This'll need rebased patches for Beta & ESR52. could anyone confirm please whether the testcase crashes on esr52?

Comment on attachment 8878242 [details] [diff] [review] patch sec-high, beta55+

(In reply to alexander :surkov from comment #17) > (In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > > This'll need rebased patches for Beta & ESR52. > > could anyone confirm please whether the testcase crashes on esr52? I can reproduce this bug on esr52.

(In reply to alexander :surkov from comment #16) > [Is this code covered by automated tests?]:fair code coverage > [Has the fix been verified in Nightly?]:yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Alexander's assessment on manual testing needs and the fact that this fix has automated coverage.

This is a compilation of two patches: this bug and bug 1369836, so asking for a review just to have to one more pair of eyes to check

Comment on attachment 8888784 [details] [diff] [review] esr patch Review of attachment 8888784 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: sec issue Fix Landed on Version: 55 Risk to taking this patch (and alternatives if risky): fair risk String or UUID changes made by this patch: no (revert nsTextFragment.h assertion change as unrelated change)

Comment on attachment 8888818 [details] [diff] [review] esr patch Fix a sec-high. Let's uplift it to ESR52.3.